Fix `nsys` subfield merging behavior #795

juntaowww · 2026-02-02T06:34:10Z

Summary

When specifying [Tests.nsys] in a scenario file to override only specific nsys configuration fields (like output), the entire nsys object was being overwritten instead of just the specified fields.

Fix:

"nsys": self.nsys.model_dump() if self.nsys else None,

to:

"nsys": self.nsys.model_dump(exclude_unset=True) if self.nsys else None,

Using exclude_unset=True ensures that only fields explicitly set by the user in the scenario file are included in the dump. The deep_merge function then correctly merges only those specified fields with the test configuration, preserving all other nsys settings from the test definition.

Test Plan

TestNsysMerging is added to tests/test_test_scenario.py for testing the correctness of overwriting of nsys fields from scenario configuration.

coderabbitai · 2026-02-02T06:34:32Z

📝 Walkthrough

Walkthrough

This PR changes nsys serialization to exclude unset fields during TestRunModel.tdef_model_dump and adds a new TestNsysMerging test suite (four tests) validating merging behavior of NsysConfiguration between base NCCLTestDefinition and scenario overrides.

Changes

Cohort / File(s)	Summary
Model Serialization `src/cloudai/models/scenario.py`	Serialize `nsys` with `model_dump(exclude_unset=True)` when present (previously default serialization); minor copyright year update.
Test Coverage `tests/test_test_scenario.py`	Added `TestNsysMerging` with four tests: partial override, multiple-field override, scenario-only addition, and disable-override behavior for `NsysConfiguration` merging; added `NsysConfiguration` to public imports used in tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hop through fields both set and free,
I hide the blanks where they shouldn't be.
Four tests I plant to check the blend,
Merge and tidy — then home I send.
✨🍃

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly summarizes the main fix: correcting nsys subfield merging behavior in scenario file overrides.
Description check	✅ Passed	The description clearly explains the bug, provides the exact code fix with before/after comparison, and documents the test plan.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

No actionable comments were generated in the recent review. 🎉

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

greptile-apps · 2026-02-02T06:36:41Z

Greptile Overview

Greptile Summary

Fixed nsys subfield merging behavior by using exclude_unset=True in model_dump() call. Previously, when users specified only specific nsys fields (like output) in a scenario file, the entire nsys object was being overwritten with default values for unset fields, causing configured fields from the base test definition to be lost.

Changed self.nsys.model_dump() to self.nsys.model_dump(exclude_unset=True) in src/cloudai/models/scenario.py:111
This ensures only explicitly set fields are included in the dump, allowing deep_merge to correctly preserve unspecified fields from the base configuration
Added comprehensive test suite TestNsysMerging with 4 test cases validating the fix for various scenarios
Updated copyright year to 2026 in both modified files

Confidence Score: 5/5

This PR is safe to merge with minimal risk
The fix is surgical (single parameter change), well-tested with comprehensive test coverage, and addresses a clear bug in the merging logic without affecting other functionality
No files require special attention

Important Files Changed

Filename	Overview
src/cloudai/models/scenario.py	Added `exclude_unset=True` to nsys model_dump() on line 111, ensuring only explicitly set fields are included during deep merge. Also updated copyright year to 2026.
tests/test_test_scenario.py	Added comprehensive test suite `TestNsysMerging` with 4 test cases covering partial overrides, multiple field overrides, adding nsys to base without nsys, and disable override scenarios. Updated copyright year to 2026.

greptile-apps

_{2 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{2 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@tests/test_test_scenario.py`:
- Around line 520-653: Remove the behavioral docstrings inside the
TestNsysMerging test methods: delete the triple-quoted strings at the top of
test_nsys_partial_override_preserves_base_config,
test_nsys_multiple_fields_override, test_nsys_scenario_adds_to_base_without_nsys
(and any similar method docstrings such as test_nsys_disable_override) so the
tests only contain assertions and setup; leave any high-level
interface/class-level documentation if needed but strip per-test behavior
descriptions that duplicate the assertions.

tests/test_test_scenario.py

greptile-apps

_{2 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

amaslenn · 2026-02-10T14:21:04Z

tests/test_test_scenario.py

+        assert tdef.nsys.enable is True
+
+    def test_nsys_multiple_fields_override(self, test_scenario_parser: TestScenarioParser, slurm_system: SlurmSystem):
+        from cloudai.core import NsysConfiguration


Can it be imported on top of this file? (same for all tests)

@juntaowww I assume you used an AI assistant to help you with the code. they really tend to bring imports deep inside. could you please share what is the assistant so we could prepare some common agents config (if there's such an option) to let the agent know about the code style?

Put them back on top of the file.

Indeed AI is used, could add some rules to prevent this in the future.

amaslenn · 2026-02-10T14:27:25Z

src/cloudai/models/scenario.py

            "cmd_args": self.cmd_args.model_dump(by_alias=by_alias) if self.cmd_args else None,
            "git_repos": [repo.model_dump() for repo in self.git_repos] if self.git_repos else None,
-            "nsys": self.nsys.model_dump() if self.nsys else None,
+            "nsys": self.nsys.model_dump(exclude_unset=True) if self.nsys else None,


Can't that lead to a dict without required fields set?

BTW, I think this is similar to what you did for reports. Maybe we should have a function merge_models(tdef, tscenario) or something like that? Do you think that will be useful?

Our current approach is "all or nothing", meaning that everything should be either set on one level or another. If we start doing smarter merge, I'd prefer we do it consistently. Overall this feels as a better and friendlier approach.

cc @podkidyshev

still learning, so my opinion should be rather optional here :)

the change seems safe and positive. agree with Andrey that it would be nice to make it consistent across all model merges but I would assume it's on the cloudai dev team's shoulders

Can't that lead to a dict without required fields set?

seems safe, if this is the risk you meant

>>> import pydantic >>> from typing import Optional >>> class A(pydantic.BaseModel): ... model_config = pydantic.ConfigDict(extra='forbid') ... a: str ... b: Optional[str] = 'b' ... >>> A.model_validate({'b': '33'}) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/ipodkidyshev/projects/cloudai/.venv/lib/python3.10/site-packages/pydantic/main.py", line 705, in model_validate return cls.__pydantic_validator__.validate_python( pydantic_core._pydantic_core.ValidationError: 1 validation error for A a Field required [type=missing, input_value={'b': '33'}, input_type=dict] For further information visit https://errors.pydantic.dev/2.11/v/missing

BTW, I think this is similar to what you did for reports. Maybe we should have a function merge_models(tdef, tscenario) or something like that? Do you think that will be useful?

Yes this is similar to the case for reports. For merge_models I think we already have similar util function deep_merge(test_defined, tc_defined)? In tdef_model_dump here I think basically it defined the overriding logic, as values in tc_defined (returned by tdef_model_dump) has higher hierarchy to override values in test_defined. (Or maybe actually we could reverse the hierarchy? But this may create chaos)

Our current approach is "all or nothing", meaning that everything should be either set on one level or another. If we start doing smarter merge, I'd prefer we do it consistently. Overall this feels as a better and friendlier approach.

Indeed agreed.

greptile-apps

_{2 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

juntaowww added 3 commits February 2, 2026 08:24

Fix nsys field merging behavior

68ffdc3

Add TestNsysMerging

2177233

Update copyright year

46da5fe

juntaowww requested review from alexmanle, amaslenn, jeffnvidia and srivatsankrishnan as code owners February 2, 2026 06:34

greptile-apps bot reviewed Feb 2, 2026

View reviewed changes

Linting

a3c6cf3

greptile-apps bot reviewed Feb 2, 2026

View reviewed changes

coderabbitai bot reviewed Feb 2, 2026

View reviewed changes

tests/test_test_scenario.py Outdated Show resolved Hide resolved

Cleanup docstrings

614bfd1

greptile-apps bot reviewed Feb 2, 2026

View reviewed changes

amaslenn reviewed Feb 10, 2026

View reviewed changes

Put imports on top of the file

bf525b0

greptile-apps bot reviewed Feb 11, 2026

View reviewed changes

Fix nsys subfield merging behavior #795

Are you sure you want to change the base?

Fix nsys subfield merging behavior #795

Conversation

juntaowww commented Feb 2, 2026

Summary

Test Plan

Uh oh!

coderabbitai bot commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

greptile-apps bot commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Overview

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

amaslenn Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

podkidyshev Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

juntaowww Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

amaslenn Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

podkidyshev Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

juntaowww Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix `nsys` subfield merging behavior #795

Fix `nsys` subfield merging behavior #795

coderabbitai bot commented Feb 2, 2026 •

edited

Loading

greptile-apps bot commented Feb 2, 2026 •

edited

Loading